Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(acns): Add advanced networking observability tls management for az aks create and update commands #7834

Merged
merged 20 commits into from
Aug 15, 2024

Conversation

rayaisaiah
Copy link
Contributor


This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

Creates commands for

az aks create --advanced-networking-observability-tls-management None
az aks create --advanced-networking-observability-tls-management Managed

az aks update --advanced-networking-observability-tls-management None
az aks update --advanced-networking-observability-tls-management Managed

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally? (pip install wheel==0.30.0 required)
  • My extension version conforms to the Extension version schema

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
You only need to update the version information in file setup.py and historical information in file HISTORY.rst in your PR but do not modify src/index.json.

Copy link

azure-client-tools-bot-prd bot commented Jul 30, 2024

⚠️Azure CLI Extensions Breaking Change Test
⚠️aks-preview
rule cmd_name rule_message suggest_message
⚠️ 1006 - ParaAdd aks create cmd aks create added parameter advanced_networking_observability_tls_management
⚠️ 1006 - ParaAdd aks update cmd aks update added parameter advanced_networking_observability_tls_management

Copy link

Hi @rayaisaiah,
Please write the description of changes which can be perceived by customers into HISTORY.rst.
If you want to release a new extension version, please update the version in setup.py as well.

Copy link

Hi @rayaisaiah,
Since the current milestone time is less than 7 days, this pr will be reviewed in the next milestone.

@yonzhan
Copy link
Collaborator

yonzhan commented Jul 30, 2024

Thank you for your contribution! We will review the pull request and get back to you soon.

@rayaisaiah rayaisaiah changed the title inital changes adding tlsmangement to cli no tests yet feat(acns): Add advanced networking observability tls management for az aks create and update commands Jul 30, 2024
Copy link

github-actions bot commented Jul 30, 2024

Copy link
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please fix failed CI checks

  1. https://dev.azure.com/azclitools/public/_build/results?buildId=177227&view=logs&j=506b8f13-8269-5210-246e-a3964e53141e&t=db7247cb-300d-5b59-fd6c-6258fad20b89&l=147

ERROR: /mnt/vss/_work/1/s/src/aks-preview/azext_aks_preview/_params.py:405:3: E121 continuation line under-indented for hanging indent
/mnt/vss/_work/1/s/src/aks-preview/azext_aks_preview/managed_cluster_decorator.py:729:1: W293 blank line contains whitespace
/mnt/vss/_work/1/s/src/aks-preview/azext_aks_preview/managed_cluster_decorator.py:732:1: W293 blank line contains whitespace
1 E121 continuation line under-indented for hanging indent
2 W293 blank line contains whitespace

  1. https://dev.azure.com/azclitools/public/_build/results?buildId=177227&view=logs&j=e2e497a5-ba27-5eb9-f39e-d929d56974a6&t=61cc2d02-f6f4-5007-71e9-5651933c3fe7&l=157
    you can add suppression rules to src/aks-preview/linter_exclusions.yml
  • FAIL - HIGH severity: option_length_too_long
    Parameter: aks create, advanced_networking_observability_tls_management - The lengths of all options ['--advanced-networking-observability-tls-management'] are longer than threshold 22. Argument advanced_networking_observability_tls_management must have a short abbreviation.
    Parameter: aks update, advanced_networking_observability_tls_management - The lengths of all options ['--advanced-networking-observability-tls-management'] are longer than threshold 22. Argument advanced_networking_observability_tls_management must have a short abbreviation.
  1. Add recording files for your newly added test cases.

name_prefix="clitest",
location="eastus",
)
def test_aks_update_advanced_networking_observability_tls_management(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe you could combine these test cases

Copy link
Contributor Author

@rayaisaiah rayaisaiah Jul 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update with the linter errors resolved and condensed create tested under one function

Copy link
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Queued live test to validate the change, test passed!

  • test_aks_update_advanced_networking_observability_tls_management
  • test_aks_create_advanced_networking_observability_tls_management

@@ -1228,6 +1228,9 @@
- name: --disable-advanced-network-observability
type: bool
short-summary: Disable advanced network observability functionalities on a cluster
- name: --advanced-networking-observability-tls-management
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be the help msg for az aks update, please also add the info for az aks create.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated and removed help messages in _params.py

@@ -3021,7 +3028,8 @@ def set_up_network_profile(self, mc: ManagedCluster) -> ManagedCluster:
if advanced_network_observability is not None:
network_profile.advanced_networking = self.models.AdvancedNetworking( # pylint: disable=no-member
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comment about AdvancedNetworking model in #7860

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for catching that updated!

Comment on lines 829 to 833
help=(
'Management of TLS certificates for querying network flow logs via the flow log endpoint.'
'Valid values are "Managed" and "None". If not specified, the default is Managed.'
)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you've written help message in _help.py, you could omit it here.

Comment on lines 1336 to 1340
help=(
'Management of TLS certificates for querying network flow logs via the flow log endpoint.'
'Valid values are "Managed" and "None". If not specified, the default is Managed.'
)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you've written help message in _help.py, you could omit it here.

@rayaisaiah
Copy link
Contributor Author

Requeued the live test pipeline to validate the updates

enabled=advanced_network_observability
# Create an advanced networking model with an observability model if it does not exist.
if network_profile.advanced_networking is None:
network_profile.advanced_networking = self.models.AdvancedNetworking( # pylint: disable=no-member
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just initialize the model in the if statement, and then assign values ​​to observability outside it, so you don't have to write the same code twice

Copy link
Contributor Author

@rayaisaiah rayaisaiah Aug 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call updated! and reran live test pipeline

Copy link
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

live test

  • test_aks_create_advanced_networking_observability_tls_management
  • test_aks_update_advanced_network_observability

Comment on lines 747 to 750
if (
enable_advanced_network_observability is not None and
enable_advanced_network_observability
):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not blocking, but I think this would be enough, enable_advanced_network_observability could a bool or None and only True would meet the condition

Suggested change
if (
enable_advanced_network_observability is not None and
enable_advanced_network_observability
):
if enable_advanced_network_observability:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated and running live test for test_aks_create_advanced_networking_observability_tls_management and
test_aks_update_advanced_network_observability

@zhoxing-ms
Copy link
Contributor

@rayaisaiah Could you please resolve this conflict?

@rayaisaiah
Copy link
Contributor Author

@rayaisaiah Could you please resolve this conflict?

updated and live test has passed for

  1. test_aks_create_advanced_networking_observability_tls_management
  2. test_aks_update_advanced_network_observability
  3. test_aks_update_enable_acns
  4. test_aks_create_with_enable_acns
  5. test_aks_create_with_enable_acns_complex

@zhoxing-ms zhoxing-ms merged commit 8929214 into Azure:main Aug 15, 2024
20 checks passed
@azclibot
Copy link
Collaborator

[Release] Update index.json for extension [ aks-preview ] : https://dev.azure.com/azclitools/release/_build/results?buildId=181512&view=results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AKS Auto-Assign Auto assign by bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants